Skip to content

address PR review feedback on the Connect Cloud auth refactor - #841

Merged
samperman merged 2 commits into
connect-cloud-auth-refactorfrom
connect-cloud-auth-refactor-pr-feedback
Aug 26, 2026
Merged

address PR review feedback on the Connect Cloud auth refactor#841
samperman merged 2 commits into
connect-cloud-auth-refactorfrom
connect-cloud-auth-refactor-pr-feedback

Conversation

@samperman

Copy link
Copy Markdown
Collaborator

Reject Connect-only deploy options for Connect Cloud.

Reject --draft so the deploy step no longer cites a Connect version for a target that has no draft step at any version.

Skip rewriting the store file when a save would not change it. DataStore._set always calls save(), so a Connect Cloud token refresh backed by the system keyring rewrote servers.json on every refresh with contents identical to what was already there,

Guard the response status against None before comparing it, since a failed connection produces a response carrying only the exception.

Trim the changelog entry, and rewrite two comments review found hard to follow.

Intent

PR feedback, will be merged into the main PR when ready: #840

Reject Connect-only deploy options for Connect Cloud.

Reject --draft so the deploy step no longer cites a
Connect version for a target that has no draft step at any version.

Skip rewriting the store file when a save would not change it.
DataStore._set always calls save(), so a Connect Cloud token refresh
backed by the system keyring rewrote servers.json on every refresh with
contents identical to what was already there,

Guard the response status against None before comparing it, since a
failed connection produces a response carrying only the exception.

Trim the changelog entry, and rewrite two comments review found hard to
follow.
@samperman
samperman requested a review from karawoo August 19, 2026 12:15
@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-26 17:50 UTC

@karawoo karawoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about the supported deploy options but otherwise lgtm

Comment thread rsconnect/validation.py
Comment on lines +65 to +72
_CONNECT_ONLY_DEPLOY_OPTIONS: dict[str, str] = {
"image": "-I/--image",
"disable_env_management": "--disable-env-management",
"env_management_py": "--disable-env-management-py",
"env_management_r": "--disable-env-management-r",
"draft": "--draft",
"metadata": "--metadata",
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should --disable-env-management-node and --node be included here too? What about --hide-all-input / --hide-tagged-input?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch on the node flags. i've added those to the list. As for the two hide flags, i think i'm going to leave those as is for now and instead investigate how to properly support them on the Connect Cloud side. I've created an issue there to track it.

A user targeting Connect Cloud via a saved nickname can reach
deploy_nodejs; reject these options the same way as the other
env-management flags.
@samperman
samperman merged commit 0cd5d6a into connect-cloud-auth-refactor Aug 26, 2026
1 check passed
@samperman
samperman deleted the connect-cloud-auth-refactor-pr-feedback branch August 26, 2026 17:49
samperman added a commit that referenced this pull request Aug 27, 2026
* feat: add support for deploying to Posit Connect Cloud

Adds Posit Connect Cloud as a deployment target alongside Posit Connect
and shinyapps.io, mirroring the R rsconnect package's support:

- Select the target with --connect-cloud or -s connect.posit.cloud.
- Authenticate with an interactive OAuth device-code login or a service
  account client ID/secret (client credentials grant), with automatic
  token refresh and write-back to the credential store.
- Register credentials with `rsconnect add`, verifying the account
  exists and grants the content:create permission before storing.
- Deploy through the Connect Cloud revision model: create or update
  content, upload the bundle to a presigned URL, publish, poll the
  revision, and print the publish log from the logs service on failure.
- Record deployments locally before publishing, since Connect Cloud
  cannot look content up by name.
- Support the production, staging, and development environments via
  CONNECT_CLOUD_ENVIRONMENT, pinned to the saved server's URL.

Fixes #817

* fix: repair Connect Cloud log authorization on Python 3.8

`cast(dict[str, Any], ...)` evaluates its first argument at runtime, so
`from __future__ import annotations` does not cover it and `dict[...]`
raises TypeError on Python 3.8. Describe the response with a TypedDict
instead, matching the other ConnectCloudClient methods.

* fix: make Connect Cloud token refresh failures actionable

ConnectCloudClient._attempt_token_refresh logged every failure at debug and
returned False, so an expired session or a revoked service account secret
surfaced only as the original opaque 401.

oauth.py now raises a typed InvalidGrantError when the token endpoint returns
error=invalid_grant, carrying the server's error_description.

The Cloud client acts on the two credential rejections it can explain:

- invalid_grant on the refresh-token path clears connect_cloud_access_token
  and connect_cloud_refresh_token on the saved servers.json entry (account
  name/id and nickname are kept) and raises "Your Posit Connect Cloud session
  has expired and could not be renewed. Authenticate again with
  `rsconnect add --connect-cloud -n <name> -A <account>`."
- invalid_client on the client-credentials path raises a message saying the
  service account credential was revoked or rotated, pointing at
  <auth host>/identity/credentials and the `rsconnect add` command with
  --client-id/--client-secret. The stored entry is left alone.

Everything else — network failures, a rejected CLI OAuth client, unexpected
responses — still returns False so the original 401 surfaces, but logs at
warning instead of debug, matching the Connect refresh path.

The servers.json write-back moved into _persist_tokens so the clearing path
reuses the field-preserving update. Connect's refresh is unchanged: its
generic `except Exception` already covers the new error type.

* feat: select Posit Connect Cloud credentials by nickname only

A saved Connect Cloud entry is a credential, not an account binding: the
login behind it can publish to every account its user has rights on. So
-A/--account no longer picks which saved credential to use, only where to
publish. With one credential saved it is used whatever account is named;
with several, -n/--name is now required and the error lists the saved
nicknames with the account each publishes to by default. This drops the
account-filter branch and its two error paths from
ServerStore._get_connect_cloud_server, along with the now-dead account_name
argument to get_by_url and resolve.

Behavior change: With several saved credentials, -n selects the credential;
-A no longer matches against entries and instead always selects the account
to publish to, so `-n cred -A other-account` publishes there with that
credential.

* feat: keep Posit Connect Cloud credentials in the system keyring

Connect Cloud tokens and service account client secrets now go to the system
keyring, keyed "<url>#<nickname>" because every Connect Cloud entry records
the same API URL. `rsconnect add` and token refresh write there when a keyring
is available and leave the matching servers.json fields out, which moves the
secrets of an entry saved before this change out of the file on its next add
or refresh. Reads prefer the keyring and fall back to those fields, so a
machine without a usable keyring (a CI runner) keeps working as before, and
`rsconnect server remove` deletes the entries for the removed nickname.
`rsconnect list` reports which of the two holds the credentials.

The keyring helpers in oauth.py now take the entry key explicitly. Posit
Connect keeps passing the bare server URL, so its "<url>:access_token" and
"<url>:refresh_token" usernames are unchanged and existing logins are
untouched. Tests get a conftest fixture that makes the keyring unavailable by
default, since the module is installed in the test environment and would
otherwise reach the machine's real keychain.

* refactor: share the 401 refresh-and-retry skeleton between both clients

RSConnectClient and ConnectCloudClient each had their own copy of "send the
request, on 401 mint a new token, send it once more". Both now inherit it from
BearerTokenHTTPServer, which calls the subclass's _attempt_token_refresh to
mint and apply the token and asks _can_refresh_token whether there is anything
to mint from -- false for an API key, a bootstrap JWT, or a Snowflake token
exchange. Connect Cloud gains the seekable-body rewind that only the Connect
copy had, so a streamed body is not sent empty on the retry.

The minting stays per-target, unchanged: Connect keeps discovery against its
registered client and the InvalidClientError re-registration recovery, and
Connect Cloud keeps the client-credentials-versus-refresh choice and its typed
error handling. The keyring-with-servers.json-fallback load and write-back is
already the same code on both sides, differing only in the key it is given;
what remains target-specific is Connect's token expiry tracking and Connect
Cloud's field-preserving write-back, which it skips for a run with no saved
entry. Connect's three copies of "find the entry this server came from" become
ServerStore.saved_entry.

No behavior change other than the added rewind; every existing test passes
unmodified.

* fix: restore Python 3.8 test collection for test_connect_cloud

The stream-body retry tests annotate returns as list[Any], which 3.8
evaluates at class-definition time and rejects. Deferring annotation
evaluation with the __future__ import fixes collection for the file.

* fix: restore fixture teardown order broken by the keyring conftest

The autouse no_system_keyring fixture requested monkeypatch, hoisting the
shared per-test instance ahead of every test-level fixture. Its undo then
ran after those fixtures' cleanup, so a test using monkeypatch.chdir into
a TemporaryDirectory had the directory deleted while it was still the
working directory, which Windows rejects (WinError 32 in
test_git_metadata teardown). The fixture now saves and restores
sys.modules itself.

* fix: tell the keyring fixture's None marker from a deleted module entry

The teardown guard treated a missing sys.modules key the same as the
fixture's own None marker, so a test that deleted the entry would raise
KeyError during restore. The sentinel default now separates the cases.

* test: cover the keyring fixture teardown branches

Extracts the fixture body into an importable generator and adds tests
driving each teardown branch: previous module restored, marker removed
when nothing was stored, and a deleted key left deleted. The marker is
reinstated through a fixture finalizer so a failing assertion cannot
leak state into later tests.

* test: cover the unknown-nickname error from ServerStore.resolve

Nothing asserted this raise; the Connect integration suite exercised it
only by accident, and the -n/-A test rework there removed even that.

* add support for -V to connect cloud deployments

* address PR review feedback on the Connect Cloud auth refactor (#841)

* address PR review feedback on the Connect Cloud auth refactor

Reject Connect-only deploy options for Connect Cloud.

Reject --draft so the deploy step no longer cites a
Connect version for a target that has no draft step at any version.

Skip rewriting the store file when a save would not change it.
DataStore._set always calls save(), so a Connect Cloud token refresh
backed by the system keyring rewrote servers.json on every refresh with
contents identical to what was already there,

Guard the response status against None before comparing it, since a
failed connection produces a response carrying only the exception.

Trim the changelog entry, and rewrite two comments review found hard to
follow.

* add --disable-env-management-node and --node to Connect-only options

A user targeting Connect Cloud via a saved nickname can reach
deploy_nodejs; reject these options the same way as the other
env-management flags.

* test: take the Connect-only deploy options from validation

The test class kept its own copy of the option list, which had already
fallen behind: --disable-env-management-node and --node were missing. It
now reads validation._CONNECT_ONLY_DEPLOY_OPTIONS, and a new test checks
each label against the flags the deploy subcommands actually declare, so
a rename in main.py can't leave the rejection message quoting a flag
nobody can pass.

* test: pin the set of Connect-only deploy options

The rejection tests derive their cases from
validation._CONNECT_ONLY_DEPLOY_OPTIONS, so deleting an entry there would
remove that option's coverage along with it. Pin the key set, so dropping
one fails here instead.

* refactor: move /v1 into the Connect Cloud logs base URL

The api field carried /v1 in the base while logs left it to the one call
site, which made the logs entries look like they were missing it. The
requests are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants